feat: [E-Commerce] Integration: PDF invoice generation#212
feat: [E-Commerce] Integration: PDF invoice generation#212
Conversation
- Create PDFInvoiceGenerator class with HTML-to-PDF conversion functionality - Update Order model with invoice generation methods and shipping fields - Add InvoiceController to handle invoice downloads - Create configuration for invoice routes and settings - Add comprehensive documentation for the PDF invoice system - Add unit tests for invoice functionality
There was a problem hiding this comment.
Pull request overview
Adds PDF invoice generation and download functionality for e-commerce orders, including a generator, routing/config, and initial tests/documentation.
Changes:
- Introduces
PDFInvoiceGeneratorwith HTML-to-PDF (DomPDF) generation and an HTML fallback. - Adds an invoice download controller + SilverStripe route configuration.
- Adds
Orderinvoice helper methods and a basic SapphireTest + fixture, plus module documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| PDFInvoiceGeneration.md | Documents invoice generation, access route, and DomPDF dependency. |
| app/src/Test/PDFInvoiceTest.yml | Adds a minimal Member fixture for invoice-related tests. |
| app/src/Test/PDFInvoiceTest.php | Adds unit tests for invoice link/filename generation and generator presence. |
| app/src/PDFInvoiceGenerator.php | Implements invoice HTML/PDF generation, an Order extension, and an invoice download controller. |
| app/src/Order.php | Introduces/updates the Order DataObject and adds invoice helper methods. |
| app/_config/pdf_invoice.yml | Wires the Order extension, adds the invoice route, and sets DomPDF defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Extension to add invoice functionality to Order model | ||
| */ | ||
| class OrderInvoiceExtension extends DataExtension | ||
| { | ||
| /** |
There was a problem hiding this comment.
OrderInvoiceExtension extends DataExtension, but DataExtension isn’t imported or fully-qualified in this file. This will fatal at runtime when the class is loaded. Add use SilverStripe\ORM\DataExtension; (or prefix with \SilverStripe\ORM\DataExtension) and consider moving the extension into its own file consistent with other extensions.
| public function download($request) { | ||
| $orderId = $request->param('ID'); | ||
|
|
||
| if (!$orderId) { | ||
| return $this->httpError(404, 'Order not found'); | ||
| } | ||
|
|
||
| $order = Order::get()->byID($orderId); | ||
|
|
||
| if (!$order || !$order->exists()) { | ||
| return $this->httpError(404, 'Order not found'); | ||
| } | ||
|
|
||
| // Generate and return the PDF invoice | ||
| return PDFInvoiceGenerator::generateInvoice($order, 'download'); | ||
| } |
There was a problem hiding this comment.
InvoiceController::download() serves invoices for any orderId without any authorisation checks. This allows any unauthenticated user to download invoices by enumerating IDs. Restrict access (e.g., require ADMIN / ORDER_MANAGE, or the currently logged-in member matching $order->MemberID, and return 403 otherwise) and consider using a signed/tokenised link rather than a predictable numeric ID.
| <div class="company-info"> | ||
| <h3>' . $companyInfo['name'] . '</h3> | ||
| <p>' . $companyInfo['address'] . '</p> | ||
| <p>' . $companyInfo['city'] . ', ' . $companyInfo['state'] . ' ' . $companyInfo['zip'] . '</p> | ||
| <p>Phone: ' . $companyInfo['phone'] . '</p> | ||
| <p>Email: ' . $companyInfo['email'] . '</p> | ||
| </div> | ||
|
|
||
| <div class="invoice-info"> | ||
| <h3>Invoice Details</h3> | ||
| <p><strong>Invoice #:</strong> ' . $order->ReferenceNumber . '</p> | ||
| <p><strong>Order Date:</strong> ' . date('M j, Y', strtotime($order->OrderDate)) . '</p> | ||
| <p><strong>Due Date:</strong> ' . date('M j, Y', strtotime($order->OrderDate . ' +30 days')) . '</p> | ||
| <p><strong>Status:</strong> ' . ucfirst($order->Status) . '</p> | ||
| </div> | ||
|
|
||
| <div class="clear"></div> | ||
|
|
||
| <div class="customer-info"> | ||
| <h3>BILL TO</h3>'; | ||
|
|
||
| if ($member && $member->exists()) { | ||
| $html .= '<p><strong>' . $member->FirstName . ' ' . $member->Surname . '</strong></p> | ||
| <p>' . $member->Email . '</p>'; | ||
| } | ||
|
|
||
| if ($order->ShippingCountry) { | ||
| $html .= '<p><strong>Shipping Address:</strong><br>'; | ||
| $html .= ($order->ShippingCity ? $order->ShippingCity . ",<br>" : ""); | ||
| $html .= ($order->ShippingState ? $order->ShippingState . " " : ""); | ||
| $html .= ($order->ShippingPostcode ? $order->ShippingPostcode . "<br>" : ""); | ||
| $html .= ($order->ShippingCountry ? $order->ShippingCountry : ""); | ||
| $html .= '</p>'; | ||
| } |
There was a problem hiding this comment.
Invoice HTML outputs user-controlled / CMS-controlled fields without escaping (e.g., company info, member names/email, and shipping address fields). This can lead to HTML injection/XSS in the fallback HTML response and can also enable SSRF when Dompdf remote fetching is enabled. Escape these values (e.g., htmlspecialchars) before embedding into HTML.
| // Output the generated PDF (1 = download, 0 = inline) | ||
| $options = [ | ||
| 'Attachment' => ($outputMode === 'download') ? 1 : 0 | ||
| ]; | ||
|
|
||
| return $dompdf->stream($fileName, $options); |
There was a problem hiding this comment.
generateWithDomPDF() always calls Dompdf::stream(), which sends output directly and doesn’t implement the documented outputMode = 'string' behavior. Consider handling modes explicitly: return raw bytes for string, and return a SilverStripe HTTPResponse with proper headers/body for inline/download instead of relying on stream().
| // Output the generated PDF (1 = download, 0 = inline) | |
| $options = [ | |
| 'Attachment' => ($outputMode === 'download') ? 1 : 0 | |
| ]; | |
| return $dompdf->stream($fileName, $options); | |
| // Get the generated PDF as a string | |
| $pdfData = $dompdf->output(); | |
| // If string output is requested, return raw PDF bytes | |
| if ($outputMode === 'string') { | |
| return $pdfData; | |
| } | |
| // Otherwise, build an HTTPResponse for inline or download | |
| $response = new HTTPResponse($pdfData); | |
| $response->addHeader('Content-Type', 'application/pdf'); | |
| $dispositionType = ($outputMode === 'download') ? 'attachment' : 'inline'; | |
| if ($fileName) { | |
| $response->addHeader( | |
| 'Content-Disposition', | |
| sprintf('%s; filename="%s"', $dispositionType, $fileName) | |
| ); | |
| } else { | |
| $response->addHeader('Content-Disposition', $dispositionType); | |
| } | |
| $response->addHeader('Content-Length', (string) strlen($pdfData)); | |
| return $response; |
| isRemoteEnabled: true | ||
| isJavascriptEnabled: true No newline at end of file |
There was a problem hiding this comment.
This config enables isRemoteEnabled and isJavascriptEnabled for Dompdf by default. With invoice HTML containing order/customer data, enabling remote resources and JS increases SSRF/XSS risk if any field is ever rendered unescaped. Prefer disabling these by default, or strictly limiting remote asset loading to trusted whitelisted domains/paths.
| isRemoteEnabled: true | |
| isJavascriptEnabled: true | |
| isRemoteEnabled: false | |
| isJavascriptEnabled: false |
|
|
||
| // Update corresponding date field | ||
| $dateField = ucfirst($newStatus) . 'Date'; | ||
| if (property_exists($this, $dateField)) { |
There was a problem hiding this comment.
updateStatus() uses property_exists($this, $dateField) to decide whether to set the corresponding *Date field. For DataObject database fields this will typically return false, so the date fields won’t be set. Use $this->hasDatabaseField($dateField) or check $this->config()->get('db') instead.
| if (property_exists($this, $dateField)) { | |
| if ($this->hasDatabaseField($dateField)) { |
| private static $db = [ | ||
| 'ReferenceNumber' => 'Varchar(100)', // Unique order reference | ||
| 'Status' => 'Varchar(20)', // pending, paid, shipped, delivered, cancelled, refunded | ||
| 'OrderDate' => 'Datetime', | ||
| 'PaidDate' => 'Datetime', | ||
| 'ShippedDate' => 'Datetime', | ||
| 'DeliveredDate' => 'Datetime', | ||
| 'CancelledDate' => 'Datetime', | ||
| 'RefundedDate' => 'Datetime', | ||
| 'TotalItems' => 'Int', | ||
| 'SubTotal' => 'Currency', | ||
| 'TaxTotal' => 'Currency', | ||
| 'ShippingTotal' => 'Currency', | ||
| 'DiscountTotal' => 'Currency', | ||
| 'GrandTotal' => 'Currency', | ||
| 'Currency' => 'Varchar(3)', // USD, EUR, etc. | ||
| 'PaymentMethod' => 'Varchar(50)', | ||
| 'ShippingMethod' => 'Varchar(50)', | ||
| 'ShippingCountry' => 'Varchar(100)', // Destination country | ||
| 'ShippingState' => 'Varchar(100)', // Destination state | ||
| 'ShippingCity' => 'Varchar(100)', // Destination city | ||
| 'ShippingPostcode' => 'Varchar(20)', // Destination postcode | ||
| 'CustomerNotes' => 'Text', | ||
| 'InternalNotes' => 'Text', | ||
| 'TrackingNumber' => 'Varchar(100)', | ||
| 'IPAddress' => 'Varchar(45)', // Support IPv6 | ||
| 'SessionID' => 'Varchar(100)' | ||
| ]; |
There was a problem hiding this comment.
PR description mentions storing invoice metadata on the order (e.g., InvoiceNumber, InvoiceGeneratedAt, InvoiceFilePath), but these fields aren’t present in Order::$db. If the intent is to persist invoice generation state and support regeneration/download, add the fields (and any relationships) here and update generation logic accordingly.
| /** | ||
| * Get the invoice download link for this order | ||
| */ | ||
| public function getInvoiceLink() | ||
| { | ||
| return '/order/invoice/' . $this->ID; | ||
| } |
There was a problem hiding this comment.
getInvoiceLink() returns a predictable URL containing only the numeric order ID. Combined with the current invoice controller, this is not a “secure download link” as described and is susceptible to enumeration. Consider adding a per-order random token (stored on the order) and validating it in the download controller, or requiring authentication + ownership checks.
| * Test that required methods exist in PDFInvoiceGenerator | ||
| */ | ||
| public function testPDFInvoiceGeneratorMethods() | ||
| { |
There was a problem hiding this comment.
These tests assume a specific execution order: testPDFInvoiceGeneratorMethods() calls method_exists('PDFInvoiceGenerator', ...) but doesn’t ensure the class is loaded/exists (PHPUnit doesn’t guarantee test order). Either assert class_exists('PDFInvoiceGenerator') within this test or instantiate the class / load via autoloader before checking methods.
| { | |
| { | |
| $this->assertTrue(class_exists('PDFInvoiceGenerator')); |
| # Add invoice extension to Order model | ||
| Order: | ||
| extensions: | ||
| - OrderInvoiceExtension | ||
|
|
There was a problem hiding this comment.
Config attaches OrderInvoiceExtension to Order, but Order already defines generateInvoice(), getInvoiceLink(), and getInvoiceFilename() directly. This makes the extension redundant (and can cause confusion about which implementation is used). Prefer keeping these methods in one place: either remove the extension/config or remove the methods from Order and rely on the extension.
| # Add invoice extension to Order model | |
| Order: | |
| extensions: | |
| - OrderInvoiceExtension |
Implements #83
Changes
Features
Fixes #83